Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.8] Handle duplicates method in eloquent collections #28194

Merged
merged 1 commit into from Apr 12, 2019
Merged

[5.8] Handle duplicates method in eloquent collections #28194

merged 1 commit into from Apr 12, 2019

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Apr 11, 2019

This PR handles the new $collection->duplicates() method for eloquent collections.

The eloquent collection method compares entities with the $a->is($b) comparison. If you think it should just compare keys $a->getKey() === $b->getKey() than I can switch it over. The only time I see this being an issue is if a collection contains two models with the same key from different tables or something like that - which I can't imagine happening - but is something to consider.

Note that the eloquent method does not alter functionality with strict mode. I've written more about this below. It currently matches how unique() method works: #28193

@timacdonald
Copy link
Member Author

Here is a bit more info on the strict / loose comparison and why: When it comes to eloquent models how should comparison work. Should eloquent models be treated differently? Because as it stands, they are.

The existing unique method bypasses strict comparison and is solely based on the keys. So if you have two different instances of a model, both with the same key...

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->unique($key = null, $strict = true);

Doing this will result in a collection that only contains $three. Which raises the question - how do we strict compare models - but also points out how Laravel currently strict compares models.

Being that unique and duplicates share a similar problem space, I think it makes sense to match existing comparison functionality.

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->duplicates($key = null, $strict = true);

Which results in a collection containing $two and $three.

If you ran unique and got the above result, and then ran duplicates and got an empty collection - those two are conflicting results for a similar problem. The following illustrates how you would end up with a collection with one unique item, but no duplicates...

$one = User::find(22);
$two = User::find(22);
$three = User::find(22);

$users = (new User)->newCollection([$one, $two, $three]);

$users->unique($key = null, $strict = true);
// [$three]

$users->duplicates($key = null, $strict = true);
// []

I think it makes sense to keep these methods in sync. If a strict comparison for unique was to be introduced, which I am personally not against, you could update this as well to match.

This illustrates the considerations you have to make when comparing eloquent models...

$userA = User::find(1);
$userA->someAttribute = 'hello';

$userB = User::find(1);
$userB->someAttribute = 'world';


$userA->getKey() === $userB->getKey(); // true

$userA->is($userB); // true

$userA == $userB; // false

$userA === $userB; // false

@taylorotwell
Copy link
Member

So is this ready to merge or you are still unsure regarding the things you mentioned above?

@timacdonald
Copy link
Member Author

I’m happy with it as is @taylorotwell

I think any strict / loose comparison change is a different conversation / PR which would target 5.9 as it would change existing behaviour for the unique method as well.

@taylorotwell taylorotwell merged commit 4b9d3e7 into laravel:5.8 Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants